Add get_aie_coredump_elf() API for generating coredump (ET_CORE) ELF - #9997
Add get_aie_coredump_elf() API for generating coredump (ET_CORE) ELF#9997sayyanna wants to merge 8 commits into
Conversation
Signed-off-by: Sri Latha Ayyannagari <SriLatha.Ayyannagari@amd.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
stsoe
left a comment
stsoe
left a comment
There was a problem hiding this comment.
Please comment the code, structs, etc.
|
One more question, who is the consumer of these new APIs? If it is XDP, I would argue that they should add this code themselves. E.g. first get the raw coredump from XRT, then re-package it using aiebu. |
larry9523
left a comment
larry9523
left a comment
There was a problem hiding this comment.
A few high level comments
- please add some detail in the PR description about why are we doing this.
- I would suggest we only provide one public API get_aie_coredump_elf() internally it will create an ELF coredump with metadata. It does not hurt to add metadata into ELF. (Thanks @stsoe for the input)
- I think there is no consumer of xrt.ini path to create aie coredump (I confirm this with @IshitaGhosh and @jvillarre). So while we keep our xrt::hw_context::get_aie_coredump() API, we should generate ELF format coredump with metadata for the xrt,ini path.
Signed-off-by: Sri Latha Ayyannagari <SriLatha.Ayyannagari@amd.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: Sri Latha Ayyannagari <SriLatha.Ayyannagari@amd.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: Sri Latha Ayyannagari <SriLatha.Ayyannagari@amd.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
| if (!ostr) | ||
| throw std::runtime_error("Could not open '" + file + "' for writing"); | ||
| ostr.write(core.data(), static_cast<std::streamsize>(core.size())); | ||
| std::abort(); |
There was a problem hiding this comment.
I think there was a mention that we should not abort on coredump, but the original spec said we should. I want to make sure remove std::abort() is indeed correct.
There was a problem hiding this comment.
We should keep the std::abort() to make sure driver clean up hw_context after timeout. We don't have to get the exception message and coredump() at one run. When usesrs set xrt.ini to save aie coredump, they already see the timeout and exception message.
Signed-off-by: Sri Latha Ayyannagari <SriLatha.Ayyannagari@amd.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: Sri Latha Ayyannagari <SriLatha.Ayyannagari@amd.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
stsoe
left a comment
stsoe
left a comment
There was a problem hiding this comment.
Looks good. Minor corrections.
Signed-off-by: Sri Latha Ayyannagari <SriLatha.Ayyannagari@amd.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
larry9523
left a comment
larry9523
left a comment
There was a problem hiding this comment.
Looks good. Some minor suggestions.
| * get_aie_coredump_elf() - Returns the coredump of AIE Array as an ET_CORE ELF. | ||
| * Fetches the raw AIE dump blob and packages it into a coredump ELF. | ||
| * The AIE architecture is derived from the ELF loaded in this context. | ||
| * Metadata (timestamp, firmware version, device info, ELF UUID) is always |
There was a problem hiding this comment.
UUID is not always present.
| // Write AIE coredump ELF to the configured file then abort. | ||
| // std::abort() ensures the OS reclaims driver resources (hw_context, hardware | ||
| // state) after a timeout. Users who set xrt.ini to capture AIE coredump | ||
| // have already observed the timeout exception on a prior run. |
There was a problem hiding this comment.
nit: "Users who..." is a regular working flow but not the code itself. You can make it clear that users would see either coredump w/ xrt.ini or exception message w/o xrt.ini
| // Each ELF has its own UUID; using the run's module ensures the | ||
| // correct UUID is embedded in the coredump metadata. | ||
| if (!m_module) | ||
| throw std::runtime_error("AIE coredump ELF not available: no ELF associated with this run"); |
There was a problem hiding this comment.
So the xrt.ini only applies to full elf flow? If so, you may want to make it clear in the comments
| get_aie_coredump() const; | ||
|
|
||
| /** | ||
| * get_aie_coredump_elf() - Returns the coredump of AIE Array as an ET_CORE ELF. |
There was a problem hiding this comment.
| * get_aie_coredump_elf() - Returns the coredump of AIE Array as an ET_CORE ELF. | |
| * get_aie_coredump_elf() - Returns the coredump of AIE Array as an ELF (e_type = ET_CORE) |
Signed-off-by: Sri Latha Ayyannagari <SriLatha.Ayyannagari@amd.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
3 participants
Problem solved by the commit
Add get_aie_coredump_elf() API for generating coredump (ET_CORE) ELF
Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered
Add get_aie_coredump_elf() API for generating coredump (ET_CORE) ELF. Currently AIE coredump is generated as a raw bin file, but this PR enables generated of coredump in ELF format. The existing get_aie_coredump() raw blob API will be deprecated.
How problem was solved, alternative solutions (if any) and why they were rejected
Added new API that package a raw AIE coredump blob into an ET_CORE ELF using the AIEBU coredump ELF writer:
std::vector
get_aie_coredump_elf(std::optionalxrt::aie::coredump_meta meta = std::nullopt) const;
The AIE architecture is derived from the ELF loaded in this context. Metadata (timestamp, versions, device info, uuid) is always embedded. If @meta is provided it is used as-is; otherwise metadata is built internally from the hw_context and device.
The coredump ELF packaging logic lives in xrt_elf.cpp and is exposed internally via xrt_core::elf_int::make_aie_coredump_elf, keeping xrt_hw_context.cpp free of any direct aiebu dependency.
Packaging the dump as an ET_CORE ELF solves:
Risks (if any) associated the changes in the commit
None
What has been tested and how, request additional testing if necessary
Added below setting in xrt.ini
[Runtime]
aie_coredump_file = aie_coredump.elf
Enabled coredump: xrt-smi configure --advanced --auto-coredump --enable
then triggered ERT_COM_STATE_TIMEOUT, coredump is ELF generated.
Documentation impact (if any)